PHSimpleVertexFinder INTT requirements#4248
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds verbosity-gated diagnostics across KFParticle code, refactors MVTX/INTT cluster checks into a shared helper, renames BCO branches in nTuples, changes DecayFinder::findDecay to return an integer count, and changes HFTrackEfficiency to compute per-daughter seed counts by iterating SvtxTrackState (seed counters default to -1). Changes
Sequence Diagram(s)(omitted — changes are diagnostic, API tweaks, and internal gating; no new multi-component sequential flow requiring diagram) Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Build & test reportReport for commit 5a29f035df4f3cc6c65db7e16ee999f06ec9ba33:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit ce03df27c0acf8e596f77aa98a7c294ba62a8d64:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 11a9d5c35e4332b28eff3ba262e6e46eec9db3d7:
Automatically generated by sPHENIX Jenkins continuous integration |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/decayfinder/DecayFinder.cc (1)
455-459:⚠️ Potential issue | 🟠 MajorDon’t zero out an accumulated decay count on a null map entry.
Line 458 returns
false, which is0for thisintfunction. If an earlierPHHepMCGenEventMapentry already incrementedreconstructableDecayWasFound, a later null entry will still makeprocess_eventsee zero and potentially abort the event.🐛 Proposed fix
if (!m_genevt) { std::cout << "DecayFinder: Missing node PHHepMCGenEvent" << std::endl; - return false; + continue; }offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.cc (1)
151-186:⚠️ Potential issue | 🟠 MajorUpdate BCO state before early event skips.
Because the BCO block now runs after the no-track/no-vertex returns, skipped events do not advance
m_prev_event_bco. The next filled candidate can write a stalelast_GL1_BCOfrom the previous accepted event rather than the previous GL1 event. Please hoist the BCO-state update before these early returns, or run it in each skip path before returning.Also applies to: 188-245, 273-274
🧹 Nitpick comments (2)
offline/packages/trackreco/PHSimpleVertexFinder.h (1)
49-52: Document downstream impact for the new INTT vertex-selection API.Please add a short compatibility note stating that defaults preserve current behavior, and whether enabling
setRequireINTT(true)changesSvtxVertexMapoutputs enough to require downstream validation or reprocessing.As per coding guidelines, “If interfaces change, ask for compatibility notes and any needed downstream updates.” Based on learnings, “If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.”
Also applies to: 96-99
offline/packages/decayfinder/DecayFinder.cc (1)
96-103: Document the analysis impact of the changed veto semantics.This now keeps an event when any reconstructable candidate is found, so accepted-event counts and saved DST contents can change relative to previous productions. Please state the expected analysis impact and whether existing samples require reprocessing. Based on learnings, “If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a20bd1d1-c22d-48cc-a032-0a680bfbe010
📒 Files selected for processing (10)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/KFParticle_sPHENIX/KFParticle_Tools.ccoffline/packages/KFParticle_sPHENIX/KFParticle_Tools.hoffline/packages/KFParticle_sPHENIX/KFParticle_eventReconstruction.ccoffline/packages/KFParticle_sPHENIX/KFParticle_nTuple.ccoffline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.ccoffline/packages/decayfinder/DecayFinder.ccoffline/packages/decayfinder/DecayFinder.hoffline/packages/trackreco/PHSimpleVertexFinder.ccoffline/packages/trackreco/PHSimpleVertexFinder.h
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Build & test reportReport for commit 51dd7c7c9e7aaefeb046e35310f7bc2c422aa9e9:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 8921a077a2c1350a53c4f66dbcdf8bb2942efd7c:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 04bbc4787c16b6adbd93fe0c91dba43cc3c895a7:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.cc (1)
381-418:⚠️ Potential issue | 🔴 CriticalOff-by-one: seed counters are left at the
-1sentinel and then incremented, undercounting by 1.
resetBranches()now initializesm_reco_track_silicon_seeds[iTrack]andm_reco_track_tpc_seeds[iTrack]to-1(the sentinel for "no reco track / not filled"), which is correct per the previous review. However, when a reco track is found we enter this block and start incrementing from-1, so a track with, say, 3 MVTX/INTT states ends up reportingsilicon_seeds = 2(and similarly 0 instead of 1 when only one silicon state exists, which is indistinguishable from the sentinel). The branches are written as/I, so this propagates directly into the nTuples and will bias downstream seed-count analyses.Reset the counters to
0once the track is confirmed, before the state loop, and keep-1strictly as the "track not reconstructed" sentinel.🔧 Proposed fix
if (recoTrackFound) { selectedTracks.insert(m_dst_track); if (Verbosity() >= VERBOSITY_MORE) { m_dst_track->identify(); } m_reco_track_exists[index] = true; m_reco_track_pT[index] = m_dst_track->get_pt(); m_reco_track_eta[index] = m_dst_track->get_eta(); m_reco_track_phi[index] = m_dst_track->get_phi(); m_reco_track_chi2nDoF[index] = m_dst_track->get_chisq() / m_dst_track->get_ndf(); + m_reco_track_silicon_seeds[index] = 0; + m_reco_track_tpc_seeds[index] = 0; for (auto state_iter = m_dst_track->begin_states(); state_iter != m_dst_track->end_states(); ++state_iter) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2b6829d-5bca-4fa8-9f9a-c8776d8fe975
📒 Files selected for processing (5)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/KFParticle_sPHENIX/KFParticle_Tools.ccoffline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.ccoffline/packages/decayfinder/DecayFinder.ccoffline/packages/trackreco/PHSimpleVertexFinder.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/KFParticle_sPHENIX/KFParticle_Tools.cc
Build & test reportReport for commit 5fd977d86e0a8488a83e67ea5357b20acb3039ba:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
reviewed
Apr 20, 2026
Contributor
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
I'm not sure why but this PR changes the pythia QA. It doesn't seem like the performance is any different but based on the code I can't see how the vertexer performance changes. @cdean-github do you have any thoughts?
Contributor
Author
I wonder if it's to do with what code rabbit picked up? I'm pushing changes based on its suggestions now. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
offline/packages/trackreco/PHSimpleVertexFinder.cc (1)
1266-1287:⚠️ Potential issue | 🔴 CriticalNull-pointer dereference when a requirement is enabled with
*_required == 0.The zero-required fix isn't fully closed. With
_nclus_required == 0,needs_clusisfalse, so the guard at L1271 is bypassed and L1276 dereferencessiliconseedunconditionally. A caller such assetRequireINTT(true)+setNinttRequired(0)combined with a track that has no silicon seed will crash — this is reachable fromcheckDCAs(L522/540/769/787), which has no pre-check on the seed.Secondary concern: even when
siliconseedis non-null but empty (or has only clusters of the other detector type),passstaysfalse, so a caller using_require_X && !passClusterRequirement(...)will still veto the track despite the user asking for zero clusters.Treating "zero required" as an automatic pass resolves both:
Proposed fix
unsigned int nclus = 0; - unsigned int _nclus_required = type == "MVTX" ? _nmvtx_required : _nintt_required; + const unsigned int nclus_required = (type == "MVTX") ? _nmvtx_required : _nintt_required; + + if (nclus_required == 0) + { + return true; + } TrackSeed *siliconseed = track->get_silicon_seed(); - bool needs_clus = _nclus_required > 0; - if (needs_clus && !siliconseed) + if (!siliconseed) { - return pass; + return false; } for (auto clusit = siliconseed->begin_cluster_keys(); clusit != siliconseed->end_cluster_keys(); ++clusit) { - uint8_t trkrId = type == "MVTX" ? TrkrDefs::mvtxId : TrkrDefs::inttId; + const uint8_t trkrId = (type == "MVTX") ? TrkrDefs::mvtxId : TrkrDefs::inttId; if (TrkrDefs::getTrkrId(*clusit) == trkrId) { - nclus++; - } - if (nclus >= _nclus_required) - { - pass = true; + if (++nclus >= nclus_required) + { + return true; + } } } + return false;As per coding guidelines,
**/*.{cc,cpp,cxx,c}changes must prioritize correctness and memory safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6db4a3e8-9885-4a3c-894c-fce268d2a237
📒 Files selected for processing (3)
offline/packages/HFTrackEfficiency/HFTrackEfficiency.ccoffline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.ccoffline/packages/trackreco/PHSimpleVertexFinder.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- offline/packages/KFParticle_sPHENIX/KFParticle_sPHENIX.cc
Build & test reportReport for commit ad8c400b45eb060180c344239ba733c2e7209adf:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 14324c07b823bca16b246db17a8ac8521caf25c7:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
|
My last commit was to fix a conflict between codeRabbit and Jenkins on what they look for (CR doesnt put braces around simple I checked the previous vertex QA and don't see any difference to the reference plots. KS results are 1.00 |
Build & test reportReport for commit 091502604916ed95144b085068b4ff2186689157:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
|
@jdosbo @pinkenburg are we OK to merge this? |
Contributor
|
I still don't really understand why the pythia QA changes because nothing in the code should change the behavior. But the change in the QA doesn't look significant, so I think we should just merge it and reset the QA |
Contributor
|
I agree, let's merge it now. |
Contributor
|
Okay, I reset the QA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
I added the ability to require INTT clusters on a track when making primary vertices. The code defaults to not requiring this, and when enabled the default is a single cluster
and
I also patched a small bug in DecayFinder which would veto events if one candidate passed the trigger but another failed
Lastly, I cleaned up some KFParticle code. Its only cosmetic
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation / Context
Add optional INTT-cluster requirements to the primary-vertex finder so vertexing can require both MVTX and INTT silicon clusters on tracks. Preserve default behavior (backwards compatible) while enabling tighter track/vertex selection when INTT information is desired. Also fix a DecayFinder logic bug that could incorrectly veto events, adjust HFTrackEfficiency seed counting, and add verbosity/diagnostic cleanups in KFParticle code.
Key changes
Potential risk areas
Possible future improvements
Caveat
AI-generated summaries can be mistaken. Review the diffs and run targeted tests to confirm behavior before merging.